feat(commonjs): support strict require semantics for circular imports #1038

lukastaegert · 2021-11-09T11:40:33Z

Rollup Plugin Name: `commonjs`

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

This includes the following breaking changes:

minimum Rollup version raised to 2.60.0 to support this.load for plugins
minimum Node version raised to 12. This allows us to use "async await" (though 10 would have been enough for that...)
if you use @rollup/plugin-node-resolve, it needs to be at least v13.0.6
Even with dynamicRequireTargets, packages that manipulate the commonjs cache like request-promise-native are no longer supported

List any relevant issue numbers:
resolves #985
resolves #988
resolves #1038
resolves #1044
resolves #1014
resolves #1082
resolves rollup/rollup#1507
resolves rollup/rollup#2747
resolves rollup/rollup#3805
resolves rollup/rollup#3816
resolves rollup/rollup#4187
resolves rollup/rollup#4195
resolves rollup/rollup#4216
resolves rollup/rollup#4231
resolves rollup/rollup#4273

Description

This introduces a new option strictRequires: "auto" | boolean | "debug" | string[] with a default of "auto". This will allow for some/all files to be wrapped in function closures so that require calls are retained as actual function calls and the first require of a module will run the module while subsequent calls will just return its current value of module.exports. true will activate it for all CommonJS files (at a small size/performance penalty), false will use the old hoisting logic for all files and you can also provide a list of mini match patterns to filter which files use the new mode.

Here is an example what this might look like for strictRequires: ['foo.js']:

// main.js
console.log(require('./foo.js));

// foo.js
exports.foo = 'bar';

// output
var foo = {};
var hasRequiredFoo; // not specifying a value is shorter than "false" for minification purposes.

function requireFoo () {
  if (hasRequiredFoo) return foo; // if this is not the first time we are importing, return exports
  hasRequiredFoo = 1; // track that we have run the module. "1" is shorter than "true" for minification purposes.
  foo.foo = 'bar';
  return foo;
}

console.log(requireFoo());

As you may already see, it already recognizes that we did not reassign module.exports and can therefore just track exports, which is called foo here re-using the module name. The variables hasRequiredFoo and requireFoo will also re-use the module name to make the code more readable. If we reassign module.exports, the code will adapt accordingly:

// main.js
console.log(require('./foo.js));

// foo.js
module.exports = 'replaced'

// output
var foo;
var hasRequiredFoo;

function requireFoo () {
  if (hasRequiredFoo) return foo;
  hasRequiredFoo = 1;
  foo = 'replaced';
  return foo;
}

console.log(requireFoo());

and if more advanced things happen, it will change further (this is mirroring the existing logic we have for hoisted modules):

// main.js
console.log(require('./foo.js));

// foo.js
if (Math.random() < 0.5) {
  module.exports = 'replaced';
} else {
  exports.foo = 'bar';
}

// output
var foo = { exports: {} };
var hasRequiredFoo;

function requireFoo () {
  if (hasRequiredFoo) return foo.exports;
  hasRequiredFoo = 1;
  if (Math.random() < 0.5) {
    foo.exports = 'replaced';
  } else {
   foo.exports.foo = 'bar';
  }
  return foo.exports;
}

console.log(requireFoo());

Why not use a shared helper to track module execution?

The implemented approach may produce slightly (but only slightly) larger results than using a shared require function. But it has some advantages:

requiring is only a single function call and thus rather fast
it can be easily statically analyzed. In the future, Rollup could possibly decide to simplify and inline/remove the wrappers again if it makes sense.
it works really well with code-splitting

What about "auto" and "debug" mode?

Here is the real magic: Having a configuration option is ok, but you do not want to learn how to use it correctly. As it turns out, nearly 100% of all issues we have result from cyclic dependencies. And with the new this.load helper, we can now figure out if a module is part of a cycle before we finished transforming the module. Thus by default, all modules that are part of a cycle will be wrapped magically and cyclic dependencies should now finally "just work" out of the box!
Note that using any value other than "auto" or "debug" for strictRequires will disable cycle detection. "debug" on the other hand works like "auto" but in the end emits a warning telling you exactly which modules were wrapped automatically so that you could copy and paste this array for manual tweaking.

What about dynamicRequireTargets?

I completely re-implemented this feature so that it is now only about dynamic requires. This should also fix some bugs related to code splitting. The new logic will no longer "register" modules, but there is a central registry for dynamic modules that just contains an object with the require functions of all dynamic modules. Modules designated as dynamic require targets are automatically wrapped in functions. The registry itself is only accessed via function helpers, and as a function is always hoisted, there should not be any issues even with complex cyclic dependencies.

Note however that the current implementation does not fully support everything dynamicRequireTargets supported before. Mainly support for require.cache is missing as this feature is no longer easily added to the logic.

Further changes

Use async-await instead of Promises in the plugin code
Meta-data from plugins returned in resolveId hooks was not attached to CommonJS modules. This is fixed now.
Infer the module type by usage for modules that have neither ESM features nor CommonJS features. I.e. if such a module is required, we assume it is CommonJS, otherwise we assume it is an ES module. This is important to decide if we should wrap if if the strictRequires option is used.
Use the options hook to inject a separate "resolver" plugin as first plugin. This is necessary as the commonjs plugin needs to be aware whenever an entry point is resolved to be able to inject an entry point ESM wrapper if strictRequires is used. Otherwise, "catch-all" resolvers like node-resolve will prevent the commonjs plugin from getting to resolve those. This is an interesting pattern for plugins that need to proxy some modules but usually just call this.resolve in their resolveId hook anyway to defer to other plugins.
When requiring an ES module with a default export that is a function, make the returned namespace callable itself to solve some interop issues.
Fix an issue where external files that were resolved to different ids by plugins were no longer treated as external. Resolves external files are being bundled for commonjs #1044 and external files are being bundled for commonjs rollup#4273
Limit the ignoreTryCatch option to external imports
Automatically wrap modules in dependency cycles, as well as modules that are required conditionally
add dynamicRequireRoot option to control which paths are stored in the bundle when using dynamic requires
throw if dynamic requires are encountered outside the dynamicRequireRoot option
make sure injected helpers never conflict with used global variables
expose plugin version on plugin object
do not transform "typeof module/module.exports/exports" for mixed modules
when using dynamicRequireTargets, ALL usages of require that are not part of a call expression are replaced with a function createCommonjsRequire('/path/to/module') so that it is possible to pass them around will still enabling them to be used. This extends to module.require as well and also includes support for require.resolve.

lukastaegert · 2021-11-09T13:23:51Z

It seems like prettier is flagging my build, which is formatted using only ESLint. Should I use prettier now instead of ESLint as they seem to be at odds here?

shellscape · 2021-11-09T13:31:53Z

pnpm lint should take care of it

lukastaegert · 2021-11-09T13:44:09Z

Unfortunately not. It finds quite a few other issues in other packages but not the ones in my package. Reason seems to be that pnpm lint is running ESLint last, while CI appears to be running pnpm prettier:check, which is not equivalent.

shellscape · 2021-11-09T13:45:28Z

I'm not sure how you are running things on your end, but we don't usually see an issue with this setup.

lukastaegert · 2021-11-09T13:53:25Z

I will just try to rewrite the code snippet in question so that ESLint and prettier do not disagree. Funnily, it seems to be that the prettier/prettier rule of ESLint disagrees with prettier...

kzc · 2021-11-10T22:20:44Z

The PR looks pretty solid.

Here's a couple of failures I encountered with this PR that worked with other bundlers and the alternative commonjs plugin from rollup/rollup#4195 (comment):

$ cat unreached.js 
0 && require("./error.js") || console.log("PASS1");

$ cat error.js 
throw new Error("FAIL");

$ node unreached.js 
PASS1

$ esbuild unreached.js --bundle | node
PASS1
 
$ node_modules/rollup/dist/bin/rollup unreached.js -p ../commonjs/dist/index.js -p node-resolve -f cjs --exports auto --silent | node
[stdin]:5
throw new Error("FAIL");
^
Error: FAIL

$ cat ms.js
const MagicString = require("magic-string"); // [email protected]
var m = new MagicString("0123456789");
console.log(m.prependRight(0, "W").prependLeft(3, "AB").appendRight(9, "XY").remove(6,8).toString());
var bundle = new MagicString.Bundle();
bundle.addSource({ filename: "foo.txt", content: m });
var map = bundle.generateMap({ file: "bundle.txt", includeContent: true, hires: true });
console.log(JSON.stringify(map));

$ node ms.js 
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ esbuild ms.js --bundle | node
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ node_modules/rollup/dist/bin/rollup ms.js -p ../commonjs/dist/index.js -p node-resolve -f cjs --exports auto --silent | node
[stdin]:1389
var m = new MagicString("0123456789");
        ^
TypeError: MagicString is not a constructor

There's also a discrepancy when strictRequireSemantic: true is used with transformMixedEsModules: true:

$ cat mixed.js 
var {hi} = require("./greetings");
hi();
import "./mod";

$ cat greetings.js 
!function(ex) {
    ex.hi = () => console.log("Hello");
}(exports);

$ cat mod.js 
console.log(require("./echo.js"));

$ cat echo.js 
module.exports = 1003;
console.log("Echo");

$ esbuild mixed.js --bundle --format=esm | node --input-type=module
Echo
1003
Hello

$ node_modules/rollup/dist/bin/rollup mixed.js -p ../commonjs/dist/index.js='{transformMixedEsModules:true}' -p node-resolve -f es --silent | node --input-type=module
Echo
1003
Hello

$ node_modules/rollup/dist/bin/rollup mixed.js -p ../commonjs/dist/index.js='{strictRequireSemantic:true,transformMixedEsModules:true}' -p node-resolve -f es --silent | node --input-type=module
Hello

An unimportant note that won't affect anyone else... As I don't use pnpm I had to apply this patch to correctly build this commonjs plugin PR with npm@6 from sources. Without this patch npm was installing [email protected] without the yet to be merged load function and was failing as result:

--- a/packages/commonjs/package.json
+++ b/packages/commonjs/package.json
@@ -66 +66 @@
-    "rollup": "^2.59.0-0",
+    "rollup": "2.59.0-0",

Edit: the mixed test was simplified.

kzc · 2021-11-11T01:14:40Z

Should the strictRequireSemantic option be renamed strictRequireSemantics since it's generally used in the plural form? Or perhaps the more succinct strictRequires or simply strict?

lukastaegert · 2021-11-11T05:18:58Z

Thanks for having a look!

Should the strictRequireSemantic option be renamed strictRequireSemantics since it's generally used in the plural form? Or perhaps the more succinct strictRequires or simply strict?

strict is probably too easily confused with other things that could be "strict" 😉

strictRequires sounds good to me as it still seems to carry the point and avoids a word that maybe not everyone understands.

The first issue you mention looks to me like it could only be solved from the plugin if we turn the feature on for all modules. I would be hesitant to do that as there is quite some impact from doing that with regard to bundle size and performance and I would regard this very much as an edge case. As long as it works correctly with strictRequires I would be happy as the idea should be that strictRequires should give you a guarantee that stuff works.

I will need to look into the other issues, that does look buggy to me.

kzc · 2021-11-11T05:45:22Z

The first issue you mention looks to me like it could only be solved from the plugin if we turn the feature on for all modules. I would be hesitant to do that as there is quite some impact from doing that with regard to bundle size and performance and I would regard this very much as an edge case

It's not uncommon to have a conditional require in debug code that is optimized away in production. Build time conditional polyfills are another example.

In any case, I am unable to get that first example working even with the strict require option.

$ node_modules/rollup/dist/bin/rollup unreached.js -p ../commonjs/dist/index.js='{strictRequireSemantic:true}' -p node-resolve -f cjs --exports auto --silent | node
[stdin]:5
throw new Error("FAIL");
^
Error: FAIL

lukastaegert · 2021-11-11T08:16:09Z

Unfortunately, it is not possible to detect conditional requires reliably even with this.load as the first entry point may use a top-level require while the second entry may use a conditional one. In such a situation, the resulting output could vary depending on which resolves faster, something we want to avoid.

I will also look why the first example does not work with strict semantics.

kzc · 2021-11-12T03:38:13Z

The commonjs circular test case in rollup/rollup#4195 (comment) also fails with this commonjs PR for the same reason that unreached.js fails above.

If any commonjs global variables (module, exports) are introduced into error.js above and cat.js then the commonjs plugin in this PR will work correctly if strict require semantics are used:

$ cat unreached2.js 
0 && require("./error2.js") || console.log("PASS2");

$ cat error2.js 
console.log("FAIL2");
exports.BLAH = 0;     // <-----------

$ node unreached2.js 
PASS2

$ node_modules/rollup/dist/bin/rollup unreached2.js -p ../commonjs='{strictRequireSemantic:true}' -p node-resolve -f esm --silent | node --input-type=module
PASS2

However, the default commonjs plugin behavior will still continue to fail:

$ node_modules/rollup/dist/bin/rollup unreached2.js -p ../commonjs -p node-resolve -f esm --silent | node --input-type=module
FAIL2
PASS2

So the obvious plugin fix would be to treat any required source file that lacks CJS or ESM keywords as if it were a CJS module for its side effects. This pattern is common for polyfills.

lukastaegert · 2021-11-12T05:51:20Z

So the obvious plugin fix would be to treat any required source file that lacks CJS or ESM keywords as if it were a CJS module for its side effects. This pattern is common for polyfills

Here the same logic applies as before: What if the file is also imported elsewhere? But as this will be a breaking release anyway, we can experiment here. So if the first "import" of a file that cannot be identified is a require, I will see if we can identify it as CommonJS.

In the end, we should eventually teach the node-resolve plugin to identify file types based on extension or package.json, at least in the node_modules folder. Then we no longer need to rely on auto-detection for those files.

lukastaegert · 2021-11-12T12:38:15Z

I renamed the option to strictRequires and added a logic that will make non-identifiable modules "commonjs" if their first import is a require.

lukastaegert · 2021-11-12T14:19:54Z

The "magic-string" issue is not a regression but "just" an interop issue. The reason is that due to the presence of the module property in magic-string's package.json, Rollup imports the ESM version of magic-string and returns its namespace, which hast the MagicString property as its default constructor. There are several ways how to solve this:

fix magic-string by providing an exports field in its package.json file that distinguishes between import and require (preferred solution)
instantiate new MagicString.default() instead of new MagicString() in the test code, accepting that you are dealing with an ESM namespace
set mainFields: ['main'] in the node-resolve plugin (thus ignoring the module property)

I can only assume esbuild works because it ignores the module property.

However it appears there might be another issue when using CommonJS with strict require semantics as entry points, looking into it.

lukastaegert · 2021-11-12T15:07:20Z

So there is an interaction with the node-resolve plugin that I did not foresee. Basically with these changes, the commonjs plugin always needs to be placed before the node-resolve plugin, otherwise CommonJS entries will not work with strictRequires. I wonder if we should just let the plugin re-order plugins in the options hook if necessary. Otherwise this will cause a lot of grief.

kzc · 2021-11-12T17:07:38Z

I can only assume esbuild works because it ignores the module property.

If memory serves, esbuild loads the CJS version of the module if it is required after applying its NodeJS module resolution rules against magic-string/package.json, just as NodeJS does:

$ node ms.js
*** magic-string.cjs.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ esbuild ms.js --bundle --format=esm | node --input-type=module
*** magic-string.cjs.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

The magic string bundles have been modified to print which version is being used.

There are several ways how to solve this:

There's another way to have a commonjs require deal with an ES module with named exports and a default export. Please see my alternative plugin implementation in rollup/rollup#4195 (comment) which also works with [email protected] whether the CJS or ESM version is requested via node-resolve's mainFields:

$ rollup ms.js -p node-resolve -p strict-cjs --silent | node --input-type=module
*** magic-string.es.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ rollup ms.js -p node-resolve='{"mainFields":["main"]}' -p strict-cjs --silent | node --input-type=module
*** magic-string.cjs.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

lukastaegert · 2021-11-12T17:39:48Z

I added a change to the plugin so that it automatically places the node-resolve plugin after commonjs if present and not yet placed correctly

kzc · 2021-11-12T18:06:44Z

It'd be ideal if the test case ms.js just worked out of the box using [email protected] without application-specific bundling options just as esbuild, webpack and rollup-plugin-strict-cjs handle it. Is there a way for @rollup/plugin-commonjs to also accomplish this with default options?

kzc · 2021-11-12T21:48:03Z

Hmm, I'm a bit baffled by this... I am unable to get the commonjs plugin in this PR to work on sources with require cycles when used with the recent version of @rollup/plugin-node-resolve specified in the latest rollup tree:

https://github.com/rollup/rollup/blob/8d98341bf746d4baa57bbd730b1fa6449555cfca/package.json#L65

No output and no errors are produced for me when using the latest version of node-resolve for applications with require cycles.

This PR's rollup-plugins/plugins/packages/commonjs/package.json is specifying a rather old version of node-resolve:

plugins/packages/commonjs/package.json

Line 63 in efd0273

"@rollup/plugin-node-resolve": "^8.4.0",

Applications with require cycles do work with this PR's commonjs plugin when using this older version of node-resolve, but not the most recent version. I'm using rollup v2.60.0 for testing. Is there anything obvious that I'm doing wrong?

kzc · 2021-11-13T06:42:08Z

An example of the node-resolve issue above using the commonjs plugin from efd0273:

$ cat one.js
console.log(module.exports = 1, require("./two"));

$ cat two.js
console.log(module.exports = 2, require("./one"));

$ node one.js
2 1
1 2

$ npm i @rollup/plugin-node-resolve@8 >/dev/null 2>&1 && npm ls | egrep 'rollup@|plugin-node-resolve' && node_modules/.bin/rollup one.js -p node-resolve -p ../commonjs --silent | node --input-type=module
├─┬ @rollup/[email protected]
├─┬ [email protected]
2 1
1 2

$ npm i @rollup/plugin-node-resolve@11 >/dev/null 2>&1 && npm ls | egrep 'rollup@|plugin-node-resolve' && node_modules/.bin/rollup one.js -p node-resolve -p ../commonjs --silent | node --input-type=module
├─┬ @rollup/[email protected]
├─┬ [email protected]
2 1
1 2

$ npm i @rollup/plugin-node-resolve@13 >/dev/null 2>&1 && npm ls | egrep 'rollup@|plugin-node-resolve' && node_modules/.bin/rollup one.js -p node-resolve -p ../commonjs --silent | node --input-type=module
├─┬ @rollup/[email protected]
├─┬ [email protected]
$      # no error, no output from rollup

lukastaegert · 2021-11-13T13:06:55Z

Unfortunately I will need to look into this on Monday. The problem was that node-resolve was preventing the plugin from proxying entry points.

kzc · 2021-11-14T16:44:18Z

The following patch adapted from rollup-plugin-strict-cjs gives @rollup/plugin-commonjs the ability to run the unmodified test ms.js correctly with the rollup defaulted ES module form of [email protected] without the need for custom plugin options:

--- a/packages/commonjs/src/helpers.js
+++ b/packages/commonjs/src/helpers.js
@@ -35,5 +35,9 @@ export function getDefaultExportFromNamespaceIfNotNamed (n) {
 export function getAugmentedNamespace(n) {
        if (n.__esModule) return n;
-       var a = Object.defineProperty({}, '__esModule', {value: true});
+       if (typeof n == "object" && typeof n.default == "function") {
+               var o = function() { return n.default.apply(this, arguments); };
+               o.prototype = n.default.prototype;
+       } else o = {__proto__: null};
+       var a = Object.defineProperty(o, '__esModule', {value: true});
        Object.keys(n).forEach(function (k) {
                var d = Object.getOwnPropertyDescriptor(n, k);

$ node_modules/rollup/dist/bin/rollup ms.js -p ../commonjs -p node-resolve -f es --silent | node --input-type=module
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

The CJS main form of [email protected] also continues to work:

$ node_modules/rollup/dist/bin/rollup ms.js -p ../commonjs -p node-resolve='{mainFields:["main"]}' -f es --silent | node --input-type=module
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

…, no longer supports require.cache) (#1038) BREAKING CHANGES: requires Node 12 No longer supports require.cache

…ion default (#1038)

…d root (#1038)

…1038)

…1038) resolves #1014

…#1038)

…orted function (requires [email protected]) (#1038) BREAKING CHANGES: Requires at least [email protected]

…tatements (#1038)

)

bhovhannes · 2022-12-14T08:42:16Z

packages/commonjs/src/helpers.js

+		};
+		a.prototype = f.prototype;
+  } else a = {};
+  Object.defineProperty(a, '__esModule', {value: true});
 	Object.keys(n).forEach(function (k) {
 		var d = Object.getOwnPropertyDescriptor(n, k);
 		Object.defineProperty(a, k, d.get ? d : {


@lukastaegert sorry for tagging you directly. I have an issue in upgrading from "@rollup/plugin-commonjs"@21.1.0 to "@rollup/plugin-commonjs"@latest. I tracked problem down to version "@rollup/plugin-commonjs"@22.0.0 and it looks like all changes in this version are in this PR. I hope you'll be able to provide valuable insights only by reading my question, that's why I tagged you.

I have a case when n which arrives to getAugmentedNamespace function already contains __esModule property.
As the if (n.__esModule) return n; line was removed by this PR, the code now throws at the Object.defineProperty(a, k, d.get ? d : { line because it tries to define __esModule property again.

Unfortunately my codebase is complex and I didn't managed to create a reproducible repo for opening an issue. Looking into exports of n it looks similar to react-dom (we use v17.0.2), but that's all I was able to find. Can you please tell me in which circumstances getAugmentedNamespace helper is added to the bundle? That may help with creating a repo to reproduce and better understanding of root cause.

How do you feel about adding back the if (n.__esModule) return n; line or modifying Object.keys(n).forEach(function (k) { line to be Object.keys(n).filter(k => k !== '__esModule').forEach(function (k) { ?

Thanks!

It is actually described in the readme https://github.com/rollup/plugins/blob/34f4c4f294b1c024552fc9674954a2d6b978e360/packages/commonjs/README.md#requirereturnsdefault in the requireReturnsDefault section, though the helper code there is not the most current one. It is only used when requireReturnsDefault is set to false or not set. It is used when importing an ES module, or when importing a module where the type cannot be determined.
But yes, that is an oversight. I guess the motivation was that __esModule is a valid ES export that would however then return a namespace object without a prototype while require usually returns an object that has the Object.prototype.
A solution could be to make __esModule configurable. One would need to look into old test cases to see if adding the __esModule short cut back breaks anything important.

Thank you very much! That was super helpful.
Adding requireReturnsDefault: 'namespace' seems to be solving issue in my case. react-dom is external in my case, so I guess this is actually what we want.
I'll try to add back a __esModule shortcut and see if PR will be green.

@lukastaegert unfortunately adding requireReturnsDefault option fixes the code for react-dom but resulted in a bad code generated to bundle some other NPM package. After a number of tries I think adding check back will be a much better fix.
I opened a MR - #1379. Only snapshot tests failed, and I reviewed them one by one and the difference was only in the line which I added back to helper. Which makes me think I didn't break something.
I'll be grateful for your review. Thanks!

Polyfill node builtins manually via individual npm packages and manual shims, to be able to update them manually to maintained versions. Notably, the buffer polyfill has been updated this way, for compatibility with the newest @ledgerhq/hw-app-btc, which uses readUint8() in BtcNew.signMessage without specifying an offset, which is not supported yet by the polyfill bundlded with rollup-plugin-polyfill-node. In order to properly handle the circular dependencies of the now updated readable-stream package, @rollup/plugin-commonjs had to be updated to at least version 22.0.0 to include rollup/plugins#1038 to fix rollup/rollup#1507. Additionally, rollup and @rollup/plugin-node-resolve had been updated to the minimum version compatible with the updated @rollup/plugin-commonjs.

lukastaegert requested a review from shellscape as a code owner November 9, 2021 11:40

lukastaegert marked this pull request as draft November 9, 2021 11:58

lukastaegert force-pushed the commonjs/strict-require-order branch 2 times, most recently from a782ae9 to e53749d Compare November 9, 2021 14:43

lukastaegert mentioned this pull request Nov 11, 2021

Plugin context function for pre-loading modules rollup/rollup#4234

Merged

9 tasks

lukastaegert force-pushed the commonjs/strict-require-order branch 3 times, most recently from 97eddef to efd0273 Compare November 12, 2021 17:35

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): reimplement dynamic import handling (requires Node 12…

493f753

…, no longer supports require.cache) (#1038) BREAKING CHANGES: requires Node 12 No longer supports require.cache

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): add strictRequires option to wrap modules (#1038)

e8dd3c3

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): automatically wrap cyclic modules (#1038)

2b3d215

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): Infer type for unidentified modules (#1038)

2aa0ac9

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): make namespace callable when requiring ESM with funct…

3c00191

…ion default (#1038)

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): limit ignoreTryCatch to external requires (#1038)

38a3aa4

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): auto-detect conditional requires (#1038)

55e7e2d

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): add dynamicRequireRoot option (#1038)

08ab82c

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): throw for dynamic requires from outside the configure…

c198d9d

…d root (#1038)

lukastaegert added a commit that referenced this pull request Apr 24, 2022

refactor(commonjs): deconflict helpers only once globals are known (#…

335a013

…1038)

lukastaegert added a commit that referenced this pull request Apr 24, 2022

feat(commonjs): expose plugin version (#1038)

1330e6a

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): do not transform "typeof exports" for mixed modules (#…

df01795

…1038) resolves #1014

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): inject module name into dynamic require function (#1038)

0b9a0e8

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): validate node-resolve peer version (#1038)

0dc11e6

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): use correct version and add package exports (#1038)

1424249

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): proxy all entries to not break legacy polyfill plugins (…

7434b0f

…#1038)

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): add heuristic to deoptimize requires after calling imp…

c583aaf

…orted function (requires [email protected]) (#1038) BREAKING CHANGES: Requires at least [email protected]

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): handle external dependencies when using the cache (#1038)

1c16a2b

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): Do not change semantics when removing requires in if s…

b1cd6a2

…tatements (#1038)

lukastaegert added a commit that referenced this pull request Apr 24, 2022

fix(commonjs): Warn when plugins do not pass options to resolveId (#1038

4c34dd5

)

kuraga mentioned this pull request Apr 24, 2022

Circular / cyclic dependency between glob.js and sync.js isaacs/node-glob#365

Closed

agilgur5 mentioned this pull request Jun 3, 2022

ReferenceError: window is not defined in 0.32.0 ezolenko/rollup-plugin-typescript2#339

Closed

wipfli mentioned this pull request Jun 4, 2022

Bump @rollup/plugin-commonjs from 21.0.3 to 22.0.0 maplibre/maplibre-gl-js#1277

Merged

niklasf added a commit to niklasf/rollup-plugins that referenced this pull request Jul 5, 2022

feat(commonjs): Restore support for node 10 after rollup#1038

288d9a8

niklasf added a commit to niklasf/rollup-plugins that referenced this pull request Jul 5, 2022

feat(commonjs): Restore support for Node 10 after rollup#1038

b39daa3

niklasf mentioned this pull request Jul 5, 2022

feat(commonjs): Restore support for Node 10 after #1038 #1219

Closed

9 tasks

sapphi-red mentioned this pull request Aug 9, 2022

refactor(resolve): remove commonjs plugin handling vitejs/vite#9460

Merged

9 tasks

bhovhannes reviewed Dec 14, 2022

View reviewed changes

bhovhannes mentioned this pull request Dec 15, 2022

fix(commonjs): produce code which works when __esModule is already defined #1379

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(commonjs): support strict require semantics for circular imports #1038

feat(commonjs): support strict require semantics for circular imports #1038

lukastaegert commented Nov 9, 2021 •

edited

Loading

lukastaegert commented Nov 9, 2021

shellscape commented Nov 9, 2021

lukastaegert commented Nov 9, 2021

shellscape commented Nov 9, 2021

lukastaegert commented Nov 9, 2021 •

edited

Loading

kzc commented Nov 10, 2021 •

edited

Loading

kzc commented Nov 11, 2021

lukastaegert commented Nov 11, 2021 •

edited

Loading

kzc commented Nov 11, 2021 •

edited

Loading

lukastaegert commented Nov 11, 2021

kzc commented Nov 12, 2021 •

edited

Loading

lukastaegert commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

kzc commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

kzc commented Nov 12, 2021 •

edited

Loading

kzc commented Nov 12, 2021

kzc commented Nov 13, 2021

lukastaegert commented Nov 13, 2021 •

edited

Loading

kzc commented Nov 14, 2021

bhovhannes Dec 14, 2022

lukastaegert Dec 14, 2022

bhovhannes Dec 14, 2022

bhovhannes Dec 15, 2022

feat(commonjs): support strict require semantics for circular imports #1038

feat(commonjs): support strict require semantics for circular imports #1038

Conversation

lukastaegert commented Nov 9, 2021 • edited Loading

Rollup Plugin Name: commonjs

Description

Why not use a shared helper to track module execution?

What about "auto" and "debug" mode?

What about dynamicRequireTargets?

Further changes

lukastaegert commented Nov 9, 2021

shellscape commented Nov 9, 2021

lukastaegert commented Nov 9, 2021

shellscape commented Nov 9, 2021

lukastaegert commented Nov 9, 2021 • edited Loading

kzc commented Nov 10, 2021 • edited Loading

kzc commented Nov 11, 2021

lukastaegert commented Nov 11, 2021 • edited Loading

kzc commented Nov 11, 2021 • edited Loading

lukastaegert commented Nov 11, 2021

kzc commented Nov 12, 2021 • edited Loading

lukastaegert commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

kzc commented Nov 12, 2021

lukastaegert commented Nov 12, 2021

kzc commented Nov 12, 2021 • edited Loading

kzc commented Nov 12, 2021

kzc commented Nov 13, 2021

lukastaegert commented Nov 13, 2021 • edited Loading

kzc commented Nov 14, 2021

bhovhannes Dec 14, 2022

Choose a reason for hiding this comment

lukastaegert Dec 14, 2022

Choose a reason for hiding this comment

bhovhannes Dec 14, 2022

Choose a reason for hiding this comment

bhovhannes Dec 15, 2022

Choose a reason for hiding this comment

lukastaegert commented Nov 9, 2021 •

edited

Loading

Rollup Plugin Name: `commonjs`

lukastaegert commented Nov 9, 2021 •

edited

Loading

kzc commented Nov 10, 2021 •

edited

Loading

lukastaegert commented Nov 11, 2021 •

edited

Loading

kzc commented Nov 11, 2021 •

edited

Loading

kzc commented Nov 12, 2021 •

edited

Loading

kzc commented Nov 12, 2021 •

edited

Loading

lukastaegert commented Nov 13, 2021 •

edited

Loading